Allow users to specify distribution and component when modifying content - #1492
Allow users to specify distribution and component when modifying content#1492daviddavis wants to merge 1 commit into
Conversation
daviddavis
changed the title
77c51ac to
80bb31b
Compare
72f3350 to
251cab2
Compare
251cab2 to
9f78c10
Compare
9f78c10 to
ca95206
Compare
|
First of all: I really appreciate this change! I started by testing some workflows using this, against my own assumptions and expectations. Overall the current state makes very reasonable design choices. It appears to work well (though I have not yet tested the content removal workflows 😉) That being said, on the details, there are several design choices I would like to have some (open ended) discussion on. To avoid this turning to chaotic, I am going to avoid GitHub's review feature for now, and will instead open a thread for each independent thing I would like to discuss. I may need a bit of time to add each thread below. |
| # A request that only names a component is scoped to the default distribution. | ||
| distribution = data.get("distribution") or PACKAGE_UPLOAD_DEFAULT_DISTRIBUTION | ||
| releases = Release.objects.filter(distribution=distribution) | ||
| repository = self.context.get("repository") | ||
| repository_version = data.get("base_version") or ( | ||
| repository.latest_version() if repository else None | ||
| ) | ||
| added = releases.filter(pk__in=data.get("add_content_units", [])).exists() | ||
| present = repository_version and releases.filter(pk__in=repository_version.content).exists() | ||
| if not (added or present): | ||
| raise DRFValidationError( | ||
| {"distribution": _("This distribution has no Release in the repository.")} | ||
| ) |
There was a problem hiding this comment.
I consider this to be the most important design choice of this new feature.
It is the thing users who want to use the new fields are most likely to trip over.
From the point of view of the publish code, we don't need to demand there be a Release object to go with the distribution we are targeting. If there is a ReleaseComponent with distribution="my-distribution", but no Releae with distribution="my-distribution", then the publish code will simply invent a default Release object: https://github.com/pulp/pulp_deb/blob/main/pulp_deb/app/tasks/publishing.py#L219
So, we could drop this requirement entirely. If users provide a distribution and/or component value to the modify endpoint, we would simply create the repo structure that these values imply. If they don't like the default values in the Release file, they could still add a custom Release object later.
However, I wonder if it makes sense to have some barrier against users accidentally messing up their repositories, for example because they have a typo in the distribution they supplied. (I suppose if you do mess it up, then the weird result would at least be contained within its own repo version which you could simply delete again).
With the current design there is a barrier against providing a weird distribution, but there is no barrier against providing a weird component. So another version of this validation might be to demand you either add or already have present a ReleaseComponent that matches both the distribution and the release you are targeting.
So I currently see three options here:
- Validation based on presence or addition of Release object (current state of the PR)
- No validation at all (you simply get what you ask for, even if what you asked for contains obvious errors).
- Validation based on presence or addition of a ReleaseComponent (I am not going to let you add the packages to a repo structure that is not already part of the repo, and that you have not explicitly asked me to add).
Opinions?
There was a problem hiding this comment.
Typos would be my biggest concern. If someone misspells something, it is not an easy action to undo. In our system, creating a release or component is an explicit step the user must perform so I think it helps to minimize this risk.
That said, we are validating that the release and the component exist before we hand the modify request off to Pulp so we don't have a strong opinion which option we decide here. In fact, I see good arguments for each.
There was a problem hiding this comment.
We discussed this one internally and we came out leaning towards "no validation", that is you get what you asked for whether that is what you wanted or not.
For number 3. we decided this would again ask users to interact with the plumbing we are trying to insulate them from.
For number 1. (validation based on Release, current state) we decided we should not demand the presence of Release objects given they are not technically required.
Overall my thinking is: If we want to move towards making use of this feature mandatory, then it must be as easy to use as possible. And throwing validation errors demanding certain things should already be in the repo (or must be provided with the modify) makes it harder to use/discourages usage.
I do still worry about typos being a very bad experience here. (Also providing one of distribution/component and not understanding about the implication that this causes the other to default to our default value). The thing that mitigates my concern is that the resulting chaos would be contained within its own repository version. Of course, if users start building further versions on top of the broken one before they realize their error, it can quickly become hard to recover from. I would hope that for most users this would only be an issue when they first experiment with the plugin, and will then quickly learn...
On the whole I still lean towards making the feature easy to use even if this increases the risk of typos resulting in bad outcomes.
|
I am going to leave it at that for now. Note that while I may have produced a lot of text, all of the above are posed as open questions. Disagreeing with any implied changes is perfectly fine. If, after some discussion, we conclude we want to stay with the current design that is a perfectly reasonable outcome. |
07bd0b6 to
f318641
Compare
|
@quba42 I think I've addressed everything. Please let me know what you think. |
|
cc @stanhu FYI as you worked on pulp/pulp-cli-deb#260 |
|
@balasankarc @stanhu reading pulp/pulp-cli-deb#245, it seems like maybe the field here should be |
quba42
left a comment
quba42
left a comment
There was a problem hiding this comment.
I found two more minor changes I would like to see.
With those final changes I am happy to merge. I have manually tested many workflows I wanted to see (like adding a package already in the repo to another ReleaseComponent, removing packages from just one ReleaseComponent. Removing a package from the repo and all ReleaseComponents (by not providing the new params). Adding multiple packages. Re-running a command that results in a noop because everything is already present, etc.
I also looked over the new automatic tests which provide good coverage for those same workflows I most wanted to test manually. 👍
| Removing a (source) package also requires removing its PackageReleaseComponent / | ||
| SourcePackageReleaseComponent links. When a distribution/component is given, the removal is | ||
| scoped to that component: a package is only removed from the repository if the scope held its | ||
| last relationship, so packages linked elsewhere or not linked at all are kept. |
There was a problem hiding this comment.
This describes my preferred behavior, and I tested this is in fact what happens. Also tested that removing a package without providing distribution and/or component, removes it and all PRCs referencing it from the repo as in the past.
So all things removal look good to me. 👍
|
One thing I did not test is what happens when you provide both add_content_units and remove_content units and a component or distribution in a single call. From the way the individual calls work and how the code is structured I am pretty confident it will simply be the conjunction of the two individual calls. I am not sure how important this corner case is. I would expect the vast majority of users to split that request up into two calls, just because the request "add these packages to this ReleaseComponent and also remove these other packages from the same ReleaseComponent at the same time" is conceptually pretty weird. 😄 If you end up requesting the same package be added and also removed in the same call, I don't particularly care what happens since such a clearly contradictory request arguably should result in undefined behavior. |
fd8123f to
516882b
Compare
|
I agree that most users will probably add/remove in a single call. It should handle requests to both add and remove content in a single call though by both adding and removing the packages that the user requests to add and remove. The one edge case you alluded to where there is an overlap of content is not obvious for users though without looking at the code (I believe that the answer is that the content gets added). I agree though that we shouldn't worry about it since it's obviously unusual and contradictory. |
38f6b71 to
a943aaf
Compare
Allow repositories/deb/apt/{pulp_id}/modify/ requests to add/remove
packages using optional distribution and component parameters. The task
will create or remove matching release structure content while
preserving package-only behavior when both parameters are omitted.
fixes pulp#1491
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c96b6a1b-7b9e-4fea-ada4-73292625182f
a943aaf to
75486ca
Compare
|
I went ahead and added a new functional test (test_add_and_remove_packages_in_same_request) to test adding/removing packages a single request. |
3 participants
Allow
repositories/deb/apt/{pulp_id}/modify/requests to add/remove packages using optionaldistributionandcomponentparameters. The task will create or remove matching release structure content while preserving package-only behavior when both parameters are omitted.fixes #1491